fix(classify): cross-address-family mapping verdict (closes #14) — v0.1.3#18
Merged
fix(classify): cross-address-family mapping verdict (closes #14) — v0.1.3#18
Conversation
Closes #14. Probes from a dual-stack network observe TWO different NATs (v4 NAT and v6 router are independent). Previously the classifier compared mapped endpoints across all successful probes for equality — cross- family endpoints can never match by construction, so any v4-literal + v6-resolved-hostname probe set was misclassified as ADM/symmetric (typical case: --server stun.l.google.com:19302 + --server <coturn-ipv4>:3478 on dual-stack network). Rework: classifyMapping now groups successful probes by Mapped.Addr().Is4(), classifies each group via the extracted classifyGroup helper, then combines via combineGroups under the rule "Unknown is absence of information, not disagreement": - both groups confident and matching → use that mapping - both confident and disagreeing → combined Unknown - one confident + one Unknown → use the confident one - both Unknown → combined Unknown CGNAT is OR'd across groups (decideForecast's CGNAT precedence then overrides forecast to Unknown). New WarnMixedAddressFamilyProbes warning added to the vocabulary, fires whenever ≥2 families present. Existing WarnADMOrStricter is suppressed under disagreement-Unknown (premise no longer holds). Other warnings (cgnat_detected, insufficient_probes) propagate independently from any group. PublicEndpoint top-level field unchanged (= first success's mapped endpoint, regardless of family). probes[] array already shows all families' endpoints. JSON schema delta is additive (new warning value).
7 new TestClassify_MixedAddressFamily cases, one per combine branch:
- mixed_v4eim_v6eim_agreed → EIM (both groups confident, match)
- mixed_v4eim_v6adm_disagreement → Unknown (both confident, differ)
- mixed_v4eim_v6singleton_v4_wins → EIM (Unknown isn't disagreement)
- mixed_v4cgnat_eim_v6eim → EIM mapping, forecast Unknown
(CGNAT precedence in decideForecast)
- mixed_singleton_each → Unknown (both groups inconclusive)
- single_family_v6_only → unchanged path, no mixed warning
- single_family_v4_unchanged_path → regression guard for existing v4 path
Plus: TestClassify_FilteringMatrix gains a final assertion that
WarnMixedAddressFamilyProbes never leaks in (matrix uses single-server
single-family probes).
Coverage stays at 95.4%.
CHANGELOG: [0.1.3] entry under Fixed (cross-family mapping bug), Added (mixed_address_family_probes warning), with migration note for JSON consumers and a note on the v0.1.2.x 4-segment-tag incident. design.md addendum: v0.1.3 marked shipped (cross-family fix), v0.1.2.x row added (asset patches between v0.1.2 and v0.1.3), hairpinning shifted to v0.1.4, natcheck server to v0.1.5. v0.2.0 line unchanged.
Contributor
Author
Code reviewNo issues found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #14.
internal/classify/classify.go: group successes byMapped.Addr().Is4(), classify each group via extractedclassifyGroup, combine viacombineGroupsunder "Unknown is absence of information, not disagreement." Newmixed_address_family_probeswarning constant (additive towarnings[]). No other schema delta.User impact: cross-family probe sets that previously emitted
nat_type: ADMnow emitUnknown(or the agreed verdict when both families match). Forecast-checking consumers (webrtc_forecast.direct_p2p) mostly unaffected — Unknown still exits 1.3 atomic commits: classifier change / 7-case test addition / CHANGELOG + design.md. Coverage 95.4%.
make test/lint/gofmt/govulncheckclean.Post-merge: re-validate against droplet with natural invocation (
stun.l.google.com:19302 + stun.cloudflare.com:3478 + 168.144.121.96:3478); tag v0.1.3.